Skip to content

Conversation

@xianwangunity
Copy link
Collaborator

Description

There are some custom styles here in input system package for two pane split views, which makes the input system setting window style appearing inconsistent compared with the rest of the editor windows. So in this PR, we simply removed the custom styles.

Testing status & QA

Manual testing.

Overall Product Risks

  • Complexity: 1
  • Halo Effect: 1

Comments to reviewers

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@unity-cla-assistant
Copy link

unity-cla-assistant commented Apr 4, 2025

CLA assistant check
All committers have signed the CLA.

@ekcoh
Copy link
Collaborator

ekcoh commented Apr 4, 2025

The CI check-ptr-title is failing since the PR title isn't prefixed "FIX: "

@xianwangunity xianwangunity changed the title [Style] Fix two pane split view custom style in input system package. FIX: Remove two pane split view custom style in input system package. Apr 4, 2025
@xianwangunity xianwangunity marked this pull request as ready for review April 4, 2025 15:42
@ekcoh
Copy link
Collaborator

ekcoh commented Apr 4, 2025

Changes so far looks good to me. Unclear why the original author applied custom style if it created I divergence from the consistent look.

Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style changes makes sense to me (have not checked it in run-time). Also needs QA approval (run-time check) before landing. Thanks for correcting this!

@xianwangunity
Copy link
Collaborator Author

Changes so far looks good to me. Unclear why the original author applied custom style if it created I divergence from the consistent look.

The original default two pane split view style in uitk was not consistent with IMGUI version. As we adopt more and more windows in uitk, this inconsistence starts to show. A lot of windows have to customize their own style to override the "wrongly" default ones.
Now that we fixed the default style in uitk, we can safely remove everywhere it's been overridden.

@LeoUnity
Copy link
Collaborator

LeoUnity commented Apr 7, 2025

Have we checked how the window looks on older Unity versions?

@Pauliusd01
Copy link
Collaborator

@xianwangunity any pointers where I should look and or what to test? I don't really notice a difference in the actions editor or project wide actions editor (project settings)

@xianwangunity
Copy link
Collaborator Author

xianwangunity commented Apr 7, 2025

@Pauliusd01 The split view borders are in different colors compared with the rest of the project setting windows. Both in dark and light themes. The borders marked in red are darker compared to the green ones.
image

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Apr 7, 2025

@xianwangunity thanks I see it now. Looks fine on trunk but 22.3 still has a different color in the project settings, maybe an editor version ifdef is needed? (not like this is a real issue anyway but if we're fixing it then we should probably do it fully)

07 04 2025 - Unity 161
07 04 2025 - Unity 162

@xianwangunity
Copy link
Collaborator Author

xianwangunity commented Apr 7, 2025

This change is coupled with this change in trunk (https://github.cds.internal.unity3d.com/unity/unity/pull/63644) and it was also backported to 6000.1, no further.

@Pauliusd01
Copy link
Collaborator

This change is coupled with this change in trunk (https://github.cds.internal.unity3d.com/unity/unity/pull/63644) and it was also backported to 6000.1, no further.

I mean I'm fine with it if you are, probably not worth to try and match the look in older versions?

@xianwangunity
Copy link
Collaborator Author

This change is coupled with this change in trunk (https://github.cds.internal.unity3d.com/unity/unity/pull/63644) and it was also backported to 6000.1, no further.

I mean I'm fine with it if you are, probably not worth to try and match the look in older versions?

I agree. Let's get this fix in asap.

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a changelog entry but I don't feel like It matters, just mentioning in case someone else disagrees. Otherwise lgtm

@xianwangunity
Copy link
Collaborator Author

@Pauliusd01 any idea why I cannot merge the branch?

@Pauliusd01
Copy link
Collaborator

@xianwangunity might be due to the unsigned Contributor License Agreement but I can merge it for you if it still doesn't work after signing

@xianwangunity
Copy link
Collaborator Author

@xianwangunity might be due to the unsigned Contributor License Agreement but I can merge it for you if it still doesn't work after signing

It's still blocked, please merge it in once you can, thanks a lot!

@Pauliusd01 Pauliusd01 merged commit 785cc8b into develop Apr 9, 2025
110 checks passed
@Pauliusd01 Pauliusd01 deleted the settings/style-fix branch April 9, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants